SCMOD-12755: Allow startup scripts to run as non-root user.#23
SCMOD-12755: Allow startup scripts to run as non-root user.#23dgibson-microfocus wants to merge 7 commits intodevelopfrom
Conversation
src/main/docker/Dockerfile
Outdated
| RUN chmod +x /startup/* /startup/startup.d/* | ||
| COPY cert-scripts/install_ca_cert.sh /startup/startup.d/install_ca_cert-base.sh | ||
| RUN chmod -R +rx /startup/* /startup/startup.d/* | ||
| COPY permissions/caf /etc/sudoers.d/caf |
There was a problem hiding this comment.
I'm don't really like the file being called caf. It doesn't project that the permissions are required for the install-ca-cert-base.sh script. Can we change the filename to install-ca-cert-base? And can we change the folder name from permissions to sudoers.d?
src/main/docker/Dockerfile
Outdated
| ADD https://raw.githubusercontent.com/CAFapi/caf-common/v1.19.0/container-cert-script/install-ca-cert.sh \ | ||
| /startup/startup.d/install-ca-cert-base.sh | ||
| RUN chmod +x /startup/* /startup/startup.d/* | ||
| COPY cert-scripts/install_ca_cert.sh /startup/startup.d/install_ca_cert-base.sh |
There was a problem hiding this comment.
The output filename is now install_ca_cert-base.sh. I don't like the mix of underscores and dash. Just keep it as install-ca-cert-base.sh like it was before. Also change the filename in the source code to match that and change the folder name from container-cert-script to startup.d, and put it inside the startup folder. Come to think of it, if it's in the startup folder like that you may not need the line at all since the COPY startup /startup line above will probably cover it.
| log "The RUNAS_USER environment variable is not set, subsequent commands will be run as the default user." | ||
| exec "$@" | ||
| fi | ||
| exec "$@" |
There was a problem hiding this comment.
Let's put the comment back so that it's back the way it was before the gosu changes.
| exec "$@" | |
| # Execute the specified command | |
| exec "$@" |
src/main/docker/permissions/caf
Outdated
| @@ -0,0 +1 @@ | |||
| ALL ALL=(ALL) NOPASSWD: /bin/cp * /etc/pki/trust/anchors*, /usr/sbin/update-ca-certificates | |||
There was a problem hiding this comment.
Is the space at the start of the line necessary? It just looks strange to me (but I don't know the syntax so keep it if it's normal).
| else | ||
| echo "Not installing CA Certificate." | ||
| fi | ||
|
|
There was a problem hiding this comment.
Drop this blank line.
dermot-hardy
left a comment
dermot-hardy
left a comment
There was a problem hiding this comment.
Does this actually work? I tried it on Moe and I'm not seeing it working:
@dermot-hardy |
|
That is a domain user so it won't exist in the |
@dermot-hardy sudo will always use your real UID for permissions checks so it must map to a user who exists on the container. Setting the |
|
Let's try to keep it simple but if we have to create a binary that wraps |
@dermot-hardy A possible alternative is we create a default non-root user in the base image and then use docker namespace remapping though I don't know how practical that would be in prod. |
|
That still leaves all containers running as the same user. It's an improvement because it's not the root user but it's still not the arbitrary user being requested here. |
4 participants
https://portal.digitalsafe.net/browse/SCMOD-12755